Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: keep normalize.css pristine #46

Merged

Conversation

huyenltnguyen
Copy link
Member

Description

Checklist:

This PR moves a CSS rule out of the normalize.css file.

The rule was added during the Modal component implementation (ref: freeCodeCamp/freeCodeCamp#54044 (comment)). But I'm looking at all of the stylesheets in the package again, I don't think we should add custom rules to normalize.css as it would make the normalize upgrade more complicated (as opposed to just copy-pasting).

The candidates to house the change are global-element-styles.css and base.css. I tried adding the code to global-element-styles.css but it didn't work, so I applied the change to base.css instead. base.css also makes more sense as the file imports Tailwind styles, and the rule we are adding is also recommended by Tailwind.

(Heads-up: I'll have more CSS cleanup coming, but I'm currently ironing things out first.)

Testing

Start Storybook and check if the Modal is displayed properly (the border is significantly thicker if the rule isn't applied).

@huyenltnguyen huyenltnguyen marked this pull request as ready for review April 9, 2024 08:58
@huyenltnguyen huyenltnguyen requested a review from a team as a code owner April 9, 2024 08:58
@ojeytonwilliams
Copy link
Contributor

I'll have more CSS cleanup coming

Is the plan to revert other changes to normalize.css in another PR? I've not been keeping track of the changes, but it seems like we've modified a few other things.

@huyenltnguyen
Copy link
Member Author

huyenltnguyen commented Apr 9, 2024

Is the plan to revert other changes to normalize.css in another PR? 've not been keeping track of the changes, but it seems like we've modified a few other things.

Cleaning up CSS stylesheets is something I'm focusing on.

Basically the goal is to export a stylesheet that would replace the bootstrap.min.css in /learn. But I've found that our component library has a little too many CSS files and they seem to override each other, so I do want to look into those and see if I can simplify / merge / clean them up.

normalize.css is a file that we copied from https://necolas.github.io/normalize.css. And I don't think we should customize the file, as it would be difficult to track the changes when we want to upgrade the version (and by "upgrade", I mean simply copy-pasting the new CSS stylesheet that the library releases). I previously added a custom rule to normalize.css, and this PR is simply to move that change elsewhere.

Related discussion: freeCodeCamp/freeCodeCamp#52030 (comment).


I honestly don't have much context on the stylesheet situation. The only stylesheet I remember adding is base.css when I set up the library, then I dashed 😆 The library has evolved from there, and I have a huge knowledge gap on why something was added or changed.

I'm currently writing an issue for the CSS stylesheets standardization / simplification to communicate what I understand so far and the strategy I propose to clean up those files. That would hopefully give you some context on the plan and the changes as well.

@ojeytonwilliams
Copy link
Contributor

And I don't think we should customize the file, as it would be difficult to track the changes when we want to upgrade the version (and by "upgrade", I mean simply copy-pasting the new CSS stylesheet that the library releases). I previously added a custom rule to normalize.css, and this PR is simply to move that change elsewhere.

Yep, 100% agree. Unfortunately I think we must have modified it more than once, because it doesn't seem to match up with https://unpkg.com/browse/[email protected]/normalize.css (even with your fixes).

@huyenltnguyen
Copy link
Member Author

huyenltnguyen commented Apr 10, 2024

Yep, that made me pull my hair.

So the thing is, in the CSS simplification work that I mentioned, I'll look into whether we need normalize.css at all.

  • If we still need normalize.css, we will want to upgrade it to the latest version (v8) before adding it to the bundle
  • If we don't need the file, we will need to review and cherry-pick the custom rules, then remove the file

This PR is a prep for both cases. The change is small / incomplete because I'm just moving my recent change–the change that I'm confident touching and have full context. And I'm not trying to go back to the official normalize.css v3, but only to the version before I touched.

@ojeytonwilliams
Copy link
Contributor

Thanks for explaining, @huyenltnguyen, that all makes sense. Also, I share your frustration!

Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@ojeytonwilliams ojeytonwilliams merged commit 53957b8 into freeCodeCamp:main Apr 10, 2024
4 checks passed
@huyenltnguyen huyenltnguyen deleted the refactor/stylesheet-cleanup branch April 11, 2024 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants